Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dispersive shift updated frequency #432

Merged
merged 4 commits into from
Jul 6, 2023

Conversation

andrea-pasquale
Copy link
Contributor

@andrea-pasquale andrea-pasquale commented Jul 6, 2023

Closes #430.

I've also renamed Frequency Shift to Chi by dividing the shift by two (hopefully it is correct).

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.

@andrea-pasquale andrea-pasquale added the bug Something isn't working label Jul 6, 2023
@andrea-pasquale andrea-pasquale self-assigned this Jul 6, 2023
@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #432 (ac8ac55) into fix_backend (5d4e2c5) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           fix_backend     #432   +/-   ##
============================================
  Coverage        96.77%   96.77%           
============================================
  Files               48       48           
  Lines             3129     3129           
============================================
  Hits              3028     3028           
  Misses             101      101           
Flag Coverage Δ
unittests 96.77% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...cal/protocols/characterization/dispersive_shift.py 100.00% <100.00%> (ø)

@rodolfocarobene
Copy link
Contributor

Tests seems to be failing for unrelated reasons, but the routine now works (and improves the fidelity!)

@andrea-pasquale
Copy link
Contributor Author

Tests seems to be failing for unrelated reasons, but the routine now works (and improves the fidelity!)

I'll have a look at tests now, thanks for the feedback.

@rodolfocarobene
Copy link
Contributor

I approved, but obviously this should be merged after the tests pass.
Also, for a future change, I think would be nice to have in the characterization section "readout frequency" AND "resonator frequency"

@andrea-pasquale
Copy link
Contributor Author

Also, for a future change, I think would be nice to have in the characterization section "readout frequency" AND "resonator frequency"

In theory we already have the information about the readout frequency in the readout pulse.
We should be more careful when updating the runcard. For this particular case I would say that the dispersive shift should only update the frequency of the readout pulse without touching the resonator frequency.
What do you think?

@rodolfocarobene
Copy link
Contributor

It makes sense, but then I would change the naming in the characterization section that is now called "readout frequency" to "resonator frequency".
Obviously doesn't change much, but now it's a bit misleading and we "loose" some information

@andrea-pasquale
Copy link
Contributor Author

It makes sense, but then I would change the naming in the characterization section that is now called "readout frequency" to "resonator frequency". Obviously doesn't change much, but now it's a bit misleading and we "loose" some information

I agree. We already have bare resonator frequency even if we are not using it in the runcards I think.
At this point we could have bare resonator frequency and dressed resonator frequency in the characterization section and we keep the readout freq in the pulse.

@Jacfomg
Copy link
Contributor

Jacfomg commented Jul 6, 2023

Also, for a future change, I think would be nice to have in the characterization section "readout frequency" AND "resonator frequency"

In theory we already have the information about the readout frequency in the readout pulse. We should be more careful when updating the runcard. For this particular case I would say that the dispersive shift should only update the frequency of the readout pulse without touching the resonator frequency. What do you think?

I think we only care about the readout frequency, we may only store the resonator frequency if we were interested on doing some simulation, right ?

Haloa

Sure rodolfo

@andrea-pasquale
Copy link
Contributor Author

I think we only care about the readout frequency, we may only store the resonator frequency if we were interested on doing some simulation, right ?

To run things yes we need only the readout frequency. I would say that there is some value in also storing the resonator frequency.

@andrea-pasquale andrea-pasquale merged commit 713cf88 into fix_backend Jul 6, 2023
@andrea-pasquale andrea-pasquale deleted the fix_dispersive_freq branch July 6, 2023 06:39
This was referenced Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants